Add WriteableStream support #9
Conversation
|
Draft for now as I haven't had time to test this yet. |
src/Web/Streams/WriteableStream.purs
Outdated
| foreign import _abort :: forall chunk. EffectFn2 (WriteableStream chunk) Error (Promise Unit) | ||
|
|
||
| abort :: forall chunk. WriteableStream chunk -> Error -> Effect (Promise Unit) | ||
| abort = runEffectFn2 _abort |
There was a problem hiding this comment.
MDN disagrees with the spec on type that should be passed to abort; MDN claims that a string should be provided, but the spec is fine with any javascript value. I went with error as that seems to be the pattern elsewhere, but I don't have a strong opinion on what should end up here.
YourFin
force-pushed
the
master
branch
2 times, most recently
from
c3c13a5 to
ae0a869
Compare
|
We refer to the spec, not what MDN says. So, I'd do whatever https://streams.spec.whatwg.org/#ws-class says. |
src/Web/Streams/WritableStream.purs
Outdated
|
|
||
| foreign import _new :: forall chunk. EffectFn2 (Sink chunk) (Nullable (QueuingStrategy chunk)) (WritableStream chunk) | ||
|
|
||
| new :: forall chunk. Sink chunk -> Maybe (QueuingStrategy chunk) -> Effect (WritableStream chunk) |
There was a problem hiding this comment.
There's 4 different ways a WriteableStream can be constructed. Could you clarify why you only support one way?
There was a problem hiding this comment.
Err I think I'm missing something here. As far as I can tell the constructor (MDN) has two options for arguments:
new WritableStream(underlyingSink)
new WritableStream(underlyingSink, queuingStrategy)
Which are covered by the Maybe on the second argument. I'm happy to cover more cases (or change the style here - right now I'm matching the ReadableStream implementation) but you'll have to point me to them.
There was a problem hiding this comment.
Ah, sorry. You support two ways to construct it via the Maybe arg.
To clarify, I'm looking at https://streams.spec.whatwg.org/#ws-class-definition:
constructor(optional object underlyingSink , optional QueuingStrategy strategy = {});
I presume that the following code would all create a WritableStream:
new WritableStream()new WritableStream(sink)new WritableStream(null, strategy)new WritableStream(sink, strategy)
src/Web/Streams/WritableStream.purs
Outdated
|
|
||
| foreign import _abort :: forall chunk. EffectFn2 (WritableStream chunk) Error (Promise Unit) | ||
|
|
||
| abort :: forall chunk. WritableStream chunk -> Error -> Effect (Promise Unit) |
There was a problem hiding this comment.
abort has an optional reason. Could you add another variant where the reason is optional? For example abort has no reason requirement whereas abortErr does?
src/Web/Streams/Sink.purs
Outdated
|
|
||
| type Optional chunk = | ||
| ( start :: WritableStreamController chunk -> Effect (Promise Unit) | ||
| , write :: WritableStreamController chunk -> chunk -> Effect (Promise Unit) |
There was a problem hiding this comment.
Shouldn't the chunk arg come before the controller arg?
There was a problem hiding this comment.
I'm happy to swap them, although I would like to know what style guide I'm missing the memo on so I can reference it in the future :)
There was a problem hiding this comment.
I don't recall the term used for it, but generally the "thing being operated on" is the last arg in a function. This allows one to use >=> to compose monadic functions (e.g. new sink strategy >=> write chunk >=> close)
The other reason is that the order done here write(stream, chunk) doesn't match the order used in the FFI / spec write(chunk, stream): https://streams.spec.whatwg.org/#underlying-sink-api
callback UnderlyingSinkWriteCallback = Promise (any chunk , WritableStreamDefaultController controller );
| import Prelude (Unit) | ||
|
|
||
| foreign import data WritableStreamController :: Type -> Type | ||
|
|
There was a problem hiding this comment.
Looks like the readonly attribute signal is missing here. If it's not added in this PR, we should open an issue tracking that.
There was a problem hiding this comment.
See my other comment on filling out the spec
There was a problem hiding this comment.
Ah; it appears that the returned AbortSignal type is currently defined in the web-fetch package, and it needs be moved to somewhere more centralized to avoid a circular dependency. A new "web-abort-signal" package seems somewhat overkill, but given its inclusion in the DOM standard rather than the fetch or stream standard I'm not sure what the best course of action would be. I'll leave this out of the next revision for now.
| write = runEffectFn2 _write | ||
|
|
||
| foreign import ready :: forall chunk. Writer chunk -> Effect (Promise Unit) | ||
|
|
There was a problem hiding this comment.
closed, desiredSize, abort, and releaseLock are all not implemented here. If you don't want to add those now, could you open an issue after this PR is merged to track that?
There was a problem hiding this comment.
I'm happy to add them; do we want to backfill the full spec on Reader too? I noticed that the implementation here is missing a few methods/properties.
There was a problem hiding this comment.
You mean https://streams.spec.whatwg.org/#default-reader-class? Perhaps as a separate PR.
Would you prefer |
|
I'd prefer |
Description of the change
Clearly and concisely describe the purpose of the pull request. If this PR relates to an existing issue or change proposal, please link to it. Include any other background context that would help reviewers understand the motivation for this PR.
Adds support for the WriteableStream api
https://developer.mozilla.org/en-US/docs/Web/API/WritableStream
Checklist: